CLDSRV-863: Checksums for PutObject and UploadPart#6105
CLDSRV-863: Checksums for PutObject and UploadPart#6105leif-scality wants to merge 7 commits intodevelopment/9.4from
Conversation
Hello leif-scality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| const { unsupportedSignatureChecksums, supportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
| // FIXME: merge this |
There was a problem hiding this comment.
The FIXME: merge this comment and // Why do we need this check... are dev notes that should be resolved or removed before merging.
— Claude Code
lib/auth/streamingV4/V4Transform.js
Outdated
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Dev note // Do we use this one or vaults? should be resolved and removed before merging.
— Claude Code
lib/routes/veeam/utils.js
Outdated
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
The // TODO comment here indicates veeam/utils.js still uses the old prepareStream which is no longer wired to checksum validation. This should be addressed in this PR or tracked explicitly.
— Claude Code
Review by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6105 +/- ##
===================================================
+ Coverage 84.38% 84.65% +0.27%
===================================================
Files 206 207 +1
Lines 13329 13518 +189
===================================================
+ Hits 11248 11444 +196
+ Misses 2081 2074 -7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
|
||
| const { unsupportedSignatureChecksums, supportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
| // FIXME: merge this |
There was a problem hiding this comment.
Debug comments left in the code: // FIXME: merge this and // Why do we need this check if we have unsupportedSignatureChecksums? appear to be development notes that should be resolved or removed before merging.
— Claude Code
lib/auth/streamingV4/V4Transform.js
Outdated
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
This comment // Do we use this one or vaults? looks like a development note that should be resolved and removed before merging.
— Claude Code
lib/routes/veeam/utils.js
Outdated
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Bare // TODO comment with no description. Either add context about what needs to be done (presumably migrating to prepareStream2) or remove if not actionable.
— Claude Code
|
| // If the x-amz-trailer header is present the request is using one of the | ||
| // trailing checksum algorithms, which are not supported. | ||
| if (headers['x-amz-trailer'] !== undefined && | ||
| if (headers['x-amz-trailer'] !== undefined && // Why do we need this check if we have unsupportedSignatureChecksums? |
There was a problem hiding this comment.
There are leftover TODO/question comments that should not be merged:
- Line 8: // Why do we need this check if we have unsupportedSignatureChecksums?
This reads as a development note, not a permanent comment. Either resolve the question or remove it before merging.
— Claude Code
lib/auth/streamingV4/V4Transform.js
Outdated
| const vault = require('../vault'); | ||
| const constructChunkStringToSign = require('./constructChunkStringToSign'); | ||
|
|
||
| // Do we use this one or vaults? |
There was a problem hiding this comment.
Leftover development comment: // Do we use this one or vaults? — resolve or remove before merging.
— Claude Code
lib/routes/veeam/utils.js
Outdated
| } | ||
| const value = Buffer.alloc(parsedContentLength); | ||
| const cbOnce = jsutil.once(callback); | ||
| // TODO |
There was a problem hiding this comment.
Bare // TODO with no description. This call still uses the old prepareStream which doesn't do checksum validation. Either migrate this to prepareStream2 or add a clear explanation of why it's deferred.
— Claude Code
| if (xAmzChecksumCnt !== 0) { | ||
| return { | ||
| error: ChecksumError.TrailerAndChecksum, | ||
| details: { trailer: headers['x-amz-trailer'], checksum: checksumHeaders }, |
There was a problem hiding this comment.
When no checksum headers are present, this returns crc64nvme as default. This causes ChecksumTransform to compute a crc64nvme hash over the entire request body for every PutObject/UploadPart, even when no checksum was requested. The digest is computed but never validated (since expected is undefined). If this is intentional (e.g. to store the computed checksum in metadata later), a comment would help clarify the intent.
— Claude Code
Review by Claude Code |
| method: 'prepareStream', | ||
| streamingV4ParamsType: typeof streamingV4Params, | ||
| streamingV4Params, | ||
| }); |
There was a problem hiding this comment.
Logging streamingV4Params at error level leaks sensitive credentials (accessKey, signatureFromRequest) into production logs. Log only safe fields like the type, not the full object.
— Claude Code
| }); | ||
| return { error: errors.InvalidArgument, stream: null }; | ||
| } | ||
| const v4Transform = new V4Transform(streamingV4Params, log, errCb); |
There was a problem hiding this comment.
In the STREAMING-AWS4-HMAC-SHA256-PAYLOAD case, there is no error event handler on v4Transform. Since .pipe() does not propagate errors, if v4Transform emits an error event (beyond what it routes through errCb internally), the error will be unhandled and checksumedStream will hang. Consider adding v4Transform.on('error', errCb) for consistency with the other transform streams in this function.
— Claude Code
There was a problem hiding this comment.
V4Transform never calls callback(err) and never emits 'error' — errors bypass Node's stream error mechanism entirely and go straight to errCb. So there's no unhandled error event
to guard against, and adding v4Transform.on('error', errCb) would be dead code.
TrailingChecksumTransform and ChecksumTransform DO need the on('error', errCb) handlers because they call callback(err) in _transform/_flush, which causes Node streams to emit
'error'. The asymmetry is intentional given the different error-routing designs.
| }; | ||
| } | ||
|
|
||
| if (this.trailerChecksumName !== `x-amz-checksum-${this.algoName}`) { |
There was a problem hiding this comment.
The validation order checks isValidDigest(expected) before verifying the trailer name matches the expected algorithm. If the trailer name does not match (e.g. x-amz-checksum-crc32 sent when crc64nvme was expected), the digest is validated against the wrong algorithm format rules, potentially returning TrailerChecksumMalformed instead of TrailerAlgoMismatch. Consider swapping the order: check trailerChecksumName match first, then validate the digest format.
— Claude Code
Review by Claude Code |
| checksumedStream.stream.removeListener('finish', onFinish); | ||
| log.error('checksum stream error after data.put', { error: err }); | ||
| cbOnce(err); | ||
| }; |
There was a problem hiding this comment.
When ChecksumTransform emits an error after data.put has already stored the object (e.g. _flush fails to compute the digest), the stored data is not cleaned up. Compare with the checksum-mismatch path at line 85 which correctly calls data.batchDelete. This handler should do the same to avoid orphaned data blobs.
— Claude Code
Review by Claude Code |
| stream.pipe(checksumedStream); | ||
| return { error: null, stream: checksumedStream }; | ||
| } | ||
| case 'UNSIGNED-PAYLOAD': // Fallthrough |
There was a problem hiding this comment.
The default case now also handles STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER (V4 signed + trailing checksum). With the new checksum logic, getChecksumDataFromHeaders would set isTrailer: true (from the x-amz-trailer header), but the chunked+signed body is piped directly into ChecksumTransform without V4 decoding or chunk-size stripping. The checksum would be computed over the raw wire format and validation would fail with TrailerMissing.
This is a pre-existing gap (old code also did not handle this content-sha256 value), but now it is more relevant since AWS SDK v3 can use this mode when both signing and checksums are enabled. Consider adding a dedicated case that chains V4Transform -> TrailingChecksumTransform -> ChecksumTransform, or returning an explicit NotImplemented error.
— Claude Code
There was a problem hiding this comment.
validateChecksumHeaders rejects STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER
|
LGTM — solid implementation of streaming checksum validation for PutObject and UploadPart. |
|
LGTM |
leif-scality
force-pushed
the
improvement/CLDSRV-863-checksums-put-object-part
branch
from
2e261da to
b3e28bd
Compare
|
LGTM |
leif-scality
force-pushed
the
improvement/CLDSRV-863-checksums-put-object-part
branch
from
b3e28bd to
5dea795
Compare
|
LGTM |
leif-scality
force-pushed
the
improvement/CLDSRV-863-checksums-put-object-part
branch
from
5dea795 to
235cd59
Compare
| }); | ||
| return { error: errors.InvalidArgument, stream: null }; | ||
| } | ||
| const v4Transform = new V4Transform(streamingV4Params, log, errCb); |
There was a problem hiding this comment.
In the STREAMING-UNSIGNED-PAYLOAD-TRAILER case (line 69), errCb is wrapped with jsutil.once() to prevent double-callback when both piped streams error. The STREAMING-AWS4-HMAC-SHA256-PAYLOAD case here does not apply the same guard — both V4Transform (via constructor) and ChecksumTransform (via .on('error', errCb)) receive the raw errCb. If both independently error, errCb would be called twice.
The current caller (storeObject.js) wraps the outer callback with jsutil.once, so this is not a bug today, but wrapping here would be more defensive and consistent with the TRAILER path.
— Claude Code
|
LGTM — solid implementation of checksum validation for PutObject and UploadPart. The new ChecksumTransform pipeline, trailer parsing, and error cleanup paths are well-structured with thorough test coverage. |
leif-scality
force-pushed
the
improvement/CLDSRV-863-checksums-put-object-part
branch
from
235cd59 to
2a22f09
Compare
| // If AWS there is a mismatch, AWS returns the same error as if the algo was invalid. | ||
| if (sdkLowerAlgo !== algo) { | ||
| return { error: ChecksumError.AlgoNotSupportedSDK, details: { algorithm: sdkAlgo } }; | ||
| } |
There was a problem hiding this comment.
Typo: "If AWS there is a mismatch" should be "If there is a mismatch".
— Claude Code
|
LGTM — solid implementation of checksum validation for PutObject and UploadPart. The stream pipeline design (V4Transform/TrailingChecksumTransform → ChecksumTransform) is clean, the mutable errCb upgrade pattern in storeObject correctly handles cleanup after data.put, and the once-guards prevent double callbacks throughout. Good test coverage across all protocol variants. |
leif-scality
deleted the
improvement/CLDSRV-863-checksums-put-object-part
branch
2 participants
No description provided.